Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Enable GNC Optimizer #1572

Merged
merged 35 commits into from
Dec 29, 2023
Merged

Enable GNC Optimizer #1572

merged 35 commits into from
Dec 29, 2023

Conversation

varunagrawal
Copy link
Collaborator

@varunagrawal varunagrawal commented Jul 10, 2023

Added our own implementation of Halley's Iteration to solve for the chi square inverse function. This allows us to use the Chi2inv function without relying on Boost, thus enabling the use of GNC as an optimizer again.

I need some feedback on the following points:
1. I have named the file with Halley's iteration as GncHelpers.h. Should I rename this to something else that is more generic? Most of the code is for computing the gamma distribution inverse, which is used to compute the chi square inverse.
2. Should the above file be within nonlinear/internal? Currently it is in nonlinear.
3. Any documentation or other aspects that would be needed? I don't expect this code to be user facing and I would welcome replacing this function with another library that is better tested and documented.

Implement chi_squared_quantile (aka the chi2inv function) using the incomplete gamma inverse function from cephes.

I added cephes to 3rdparty and added the necessary CMake files to build and link against GTSAM. This makes the change for us really simple.

@varunagrawal varunagrawal self-assigned this Jul 10, 2023
@varunagrawal varunagrawal requested review from dellaert and removed request for jingnanshi July 10, 2023 18:49
@lucacarlone
Copy link
Contributor

@varunagrawal : this looks excellent. I'll leave the question of (re)naming of GncHelper to @dellaert . My only comment/suggestion would be to add a few more unit tests. This is a quite basic functionality and it we get it wrong, it might be very hard to debug in downstream applications.

Copy link
Member

@dellaert dellaert left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome!. I'm fine with the location of the file. Agreed with Luca on unit tests, but my main request is to add some information in GncHelpers as to where this code comes from? Is this de-novo code? Was it copied from boost? Also, if it only implements chi-square, why not name it as such? Finally, this is not a class I think so the file should start with a lowercase letter.

@varunagrawal
Copy link
Collaborator Author

I looked at various implementations from both Boost and GCEM to understand how Halley's algorithm worked and how to deal with different datatypes effectively, but I wrote this pretty much on my own.

After some thought, I am considering these changes:

  1. Rename GncHelpers to chiSquaredInverse.hpp.
  2. Move it to nonlinear/internal since it is mostly for internal usage.
  3. Unit test each individual function. I read up on how Boost does this and will do similar things.
  4. Improve docs as I go along.

@dellaert
Copy link
Member

I looked at various implementations from both Boost and GCEM to understand how Halley's algorithm worked and how to deal with different datatypes effectively, but I wrote this pretty much on my own.

After some thought, I am considering these changes:

  1. Rename GncHelpers to chiSquaredInverse.hpp.
  2. Move it to nonlinear/internal since it is mostly for internal usage.
  3. Unit test each individual function. I read up on how Boost does this and will do similar things.
  4. Improve docs as I go along.

Sounds good. Please do document the above. Having straight copies of code from boost where possible would be preferable as those functions would not have to be unit-tested, and it is acceptable under their license.

@dellaert
Copy link
Member

...with attribution, of course

Copy link
Member

@dellaert dellaert left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks !!!

@varunagrawal
Copy link
Collaborator Author

I wanted to give a quick update on this:

  1. Adding code from Boost was getting really unwieldy, resulting in copy-paste of over a dozen files for computing the incomplete gamma function (this is used to compute the chi-squared inverse easily). This code was also getting far too complicated for long-term maintenance.
  2. I did some looking around and found cephes which gives us everything we need in a really lightweight C library. Moreover scipy uses cephes for their implementation of the incomplete gamma inverse function so it is production grade.
  3. Cephes is released under a very liberal license which makes bundling it under 3rdparty really easy.
  4. As a result, I added cephes to 3rdparty and got it compiling. The overall PR now is much simpler with much of the work being offloaded to cephes.

Copy link
Contributor

@lucacarlone lucacarlone left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@varunagrawal varunagrawal merged commit 5c87f97 into develop Dec 29, 2023
30 checks passed
@varunagrawal varunagrawal deleted the chi-squared-quantile branch December 29, 2023 22:26
@varunagrawal
Copy link
Collaborator Author

Cephes license can be found here: https://smath.com/en-US/view/CephesMathLibrary/license

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants